Skip to content

Support telescope position from EarthLocations for SubarrayDescription#2424

Draft
StFroese wants to merge 1 commit intomainfrom
support_earth_location
Draft

Support telescope position from EarthLocations for SubarrayDescription#2424
StFroese wants to merge 1 commit intomainfrom
support_earth_location

Conversation

@StFroese
Copy link
Member

fixes #2162

@StFroese StFroese force-pushed the support_earth_location branch from 25fc030 to cdd9b8d Compare October 25, 2023 15:14
@StFroese StFroese requested a review from maxnoe October 25, 2023 15:16
@StFroese StFroese force-pushed the support_earth_location branch from cdd9b8d to 3acdd9b Compare October 26, 2023 13:53
@StFroese
Copy link
Member Author

Ignore the test for now, I'll write a new one as a regression test...

tel_positions : Dict[int, np.ndarray]
dict of x,y,z telescope positions on the ground by tel_id. These are
tel_positions : Dict[int, Union[np.ndarray, EarthLocation]]
dict of x,y,z telescope positions on the ground by tel_id or EarthLocation. These are
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by tel_id or EarthLocation sounds like EarthLocation could be the key of the dictionary, not another format for the value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, better with something like

dict of telescope positions by tel_id, given as x,y,z position or EarthLocation.

"""
self.name = name
self.positions = tel_positions or dict()
self.positions: Dict[int, Union[np.ndarray, EarthLocation]] = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use mixed types for internal storage. We should accept EarthLocation or GroundFrame coordinates for the inputs, but then convert in __init__ to one common format.

I think for now, that should be the previous, so ground frame coordinas.

pos_x = [p[0].to_value(u.m) for p in self.positions.values()]
pos_y = [p[1].to_value(u.m) for p in self.positions.values()]
pos_z = [p[2].to_value(u.m) for p in self.positions.values()]
positions = self.positions.values()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, internal representation should not be mixed.

@maxnoe maxnoe marked this pull request as draft November 17, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SubarrayDescription should support taking telescope positions as EarthLocation

3 participants